-
Notifications
You must be signed in to change notification settings - Fork 783
refactor: replace moment with dayjs #1856
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @hyperz111, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the project's date handling by replacing the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Greptile OverviewGreptile SummaryThis PR replaces the unmaintained The main behavioral risk is in Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Plugin view (plugin.view.js)
participant Dayjs as dayjs + plugins
UI->>Dayjs: extend(relativeTime)
UI->>Dayjs: extend(updateLocale)
UI->>UI: render Plugin page
UI->>UI: formatUpdatedDate(packageUpdatedAt)
UI->>Dayjs: updateLocale("en", relativeTime overrides)
UI->>Dayjs: dayjs(dateString)
Dayjs-->>UI: Dayjs instance
UI->>Dayjs: isValid()
Dayjs-->>UI: boolean
UI->>Dayjs: fromNow(true)
Dayjs-->>UI: relative string
UI-->>UI: display "(updatedText)" in Version component
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 2 comments
Additional Comments (1)
In Also, A minimal fix is to use Prompt To Fix With AIThis is a comment left during a code review.
Path: src/pages/plugin/plugin.view.js
Line: 52:80
Comment:
[P1] `fromNow(true)` drops the “ago/in …” suffix, changing UI text
In `formatUpdatedDate`, `moment.utc(dateString).fromNow()` previously returned strings including the suffix (e.g., `"5m ago"`). With Day.js you’re currently doing `updateTime.fromNow(true)`, which returns *only* the distance (e.g., `"5m"`) and will never append `past`/`future` strings (so the `past: "%s ago"` config won’t apply). This changes the displayed `packageUpdatedAt` text.
Also, `moment.utc(dateString)` preserved UTC semantics; `dayjs(dateString)` will parse in local time unless the input includes a timezone offset. If `package_updated_at` is UTC and doesn’t include an offset, this can skew the relative time.
A minimal fix is to use `fromNow()` (no arg) and, if UTC is required, `dayjs.utc(dateString)` with the `utc` plugin.
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request successfully replaces the unmaintained moment library with dayjs. The dependency changes in package.json and package-lock.json are correct. However, I've identified a critical issue in src/pages/plugin/plugin.view.js where replacing moment.utc() with dayjs() could lead to incorrect date parsing due to timezone differences. I've also pointed out an opportunity to improve performance and fix a minor display bug by moving the dayjs locale configuration out of a frequently called function. Overall, a good refactoring with a couple of important adjustments needed.
|
While Moment.js is unmaintained it's API is pretty much good considerably. The Moment.js is a Pretty Much Done on its base Feature set. Also, Moment.js explained this better in their docs here -> https://momentjs.com/docs/#/-project-status/ |
|
I get the |
|
I don't think there is need for this change. |
I get this build size difference. Before: After: |
@bajrangCoder, I think you should read https://momentjs.com/docs/#/-project-status/ before making a decision, also considering the package of moment.js (~4 mb) and dayjs being <1 mb |
momentis unmaintained, so i replace it withdayjs.